-
Notifications
You must be signed in to change notification settings - Fork 215
Implementing product type constants #4474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me! I have one suggestion for the changelog entry for clarity, and a non-blocking comment that is driven by a personal style preference so doesn't need action.
@@ -92,7 +95,7 @@ public function ajax_add_to_cart() { | |||
// First empty the cart to prevent wrong calculation. | |||
WC()->cart->empty_cart(); | |||
|
|||
if ( ( 'variable' === $product_type || 'variable-subscription' === $product_type ) && isset( $_POST['attributes'] ) ) { | |||
if ( ( ProductType::VARIABLE === $product_type || 'variable-subscription' === $product_type ) && isset( $_POST['attributes'] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very much non-blocking personal preference (which may be worth discussing as a team): I prefer using fully-qualified references inline as much as possible, mostly because it's not always easy to understand what the class is when viewing isolated sections of the code, especially via diffs.
if ( ( ProductType::VARIABLE === $product_type || 'variable-subscription' === $product_type ) && isset( $_POST['attributes'] ) ) { | |
if ( ( Automattic\WooCommerce\Enums\ProductType::VARIABLE === $product_type || 'variable-subscription' === $product_type ) && isset( $_POST['attributes'] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! To add to this, I found a page with a nice discussion about it: https://www.sitepoint.com/community/t/using-namespace-prefix-versus-use-statement/351357/11
Co-authored-by: daledupreez <dale.du.preez@automattic.com>
📈 PHP Unit Code Coverage Report
|
Changes proposed in this Pull Request:
Since WC 9.7 is our current minimum version, we can implement the new constants for product types.
Testing instructions
Code review. Check if the tests are still passing.
Changelog entry
Changelog Entry Comment
Comment
Post merge